Revert "fix(elasticsearch): skip bootstrap-only checks when joining existing cluster (#149)"#150
Merged
Merged
Conversation
…xisting cluster (#149)" This reverts commit 39037c0. The block-level `when: not elasticsearch_cluster_set_up | bool` on the "Runtime cluster setup" block in elasticsearch-security.yml regresses fresh-install scenarios. The block contains an inner set_fact that flips elasticsearch_cluster_set_up to true partway through, after the initial passwords file is written. Block-level when is copied to every inner task and re-evaluated against the current variable state, so every task after that internal set_fact silently skips — including the "Fetch elastic password after initial setup" task that sets elasticstack_password. Downstream consumers (cluster settings, watermarks, the molecule shared set_ci_watermarks helper) then see elasticstack_password undefined and either fall back to a hardcoded file path that doesn't exist on custom-path scenarios, or fail outright. Symptom in CI: elasticsearch_custom, elasticsearch_cert_content, and elasticsearch_diagnostics scenarios fail at the watermark setup step on rocky9/debian13 + ES 9. Previously green on the same scenarios. Reverting both files together. The main.yml master-count change is reverted alongside it because it's only useful in combination with the join-existing-cluster path that needs further work to land cleanly. A follow-up PR will gate only the bootstrap-password tasks individually instead of the whole block. Reopens #148.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR removes conditional guards that depended on ChangesRemove Cluster Setup State Guards
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Oddly
added a commit
that referenced
this pull request
May 7, 2026
…cluster (#151) Adds `not elasticsearch_cluster_set_up | bool` to the four bootstrap-password tasks in elasticsearch-security.yml plus the master-count check in main.yml, so a joining node skips the checks that only make sense for fresh-bootstrap. Reopens issue #148 after the previous attempt (#149, reverted in #150) regressed fresh-install scenarios. The previous attempt put the gate on the whole "Runtime cluster setup" block. That block contains an inner `set_fact` that flips elasticsearch_cluster_set_up to true after the initial passwords file is written. Ansible re-evaluates block-level whens per task against the live variable, so every inner task after that flip silently skipped, including the fetch that sets elasticstack_password. Downstream tasks then saw it undefined and either fell back to a hardcoded path that doesn't exist on custom-path scenarios, or failed outright. Gating per task avoids that — each task's when is independent and doesn't get re-evaluated against later state. The four gated tasks all already had `not elasticsearch_passwords_file.stat.exists | bool` in their when, so they're a clean place for the additional guard. The master-count check in main.yml was uncontroversial in #149 — it sits before the security tasks run and the inner flip never reaches it — so it returns unchanged. No molecule scenario covers the join-existing-cluster path: it would need a multi-node setup where the joining node sees the passwords file as absent (stat delegated to the CA host), which the existing single-node scenarios can't reproduce. Existing scenarios still validate the fresh-bootstrap path and would catch a regression like the one #149 introduced.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reverts #149.
The block-level
when: not elasticsearch_cluster_set_up | boolon the "Runtime cluster setup" block inelasticsearch-security.ymlregresses fresh-install scenarios. The block contains an innerset_factthat flipselasticsearch_cluster_set_uptotruepartway through, after the initial passwords file is written. Ansible copies a block-levelwhenonto every inner task and re-evaluates it against the current variable state, so every task after that internal set_fact silently skips — includingFetch elastic password after initial setup, which is what setselasticstack_password. Downstream consumers (cluster settings, watermarks, the molecule sharedset_ci_watermarkshelper) then seeelasticstack_passwordundefined and either fall back to a hardcoded path that doesn't exist for custom-path scenarios, or fail outright.I caught this on a workflow_dispatch of
Test Role elasticsearchagainst main after the merge:elasticsearch_custom,elasticsearch_cert_content, andelasticsearch_diagnosticsfailed at the watermark step on rocky9 + ES 9 (and debian13 for diagnostics). The same scenarios were green on the schedule run from yesterday, and main is exactly +2 commits since then (#147 + #149), confirming the regression source.I'm reverting both files in #149 together. The
main.ymlmaster-count change isn't broken by itself, but it's only useful in combination with the join-existing-cluster path, so it goes back along with the security-tasks revert and returns in the follow-up PR.A follow-up will replace this with task-level gating: add
not elasticsearch_cluster_set_up | boolto the individual bootstrap-password tasks (Check for API with bootstrap password, the recovery sub-block, theFailtask, andCheck for cluster status with bootstrap password) so they skip on a joining node without poisoning the inner set_fact. Reopens #148.Summary by CodeRabbit
Release Notes